Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1184 allow delete-only saas endpoints #1200

Merged
merged 3 commits into from
Oct 10, 2022

Conversation

galvana
Copy link
Contributor

@galvana galvana commented Oct 3, 2022

Closes #1184

Code Changes

  • Modified SaaSConfig.get_graph() to add a placeholder field that assists with graph traversal when a delete request is defined without a read request
  • Modified SaaSConnector.retrieve_data() to return a single empty dictionary [{}] in the event of a delete-only endpoint to still be able to trigger SaaSConnector.mask_data()
  • Updated the SaaS config tests
  • Updated the SaaS connector tests

Steps to Confirm

  • Covered by unit tests

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Updates the dataset that is generated from a SaaS config. Originally, we used the param_values of a read request to generate the collections and the identity/dataset references. We needed to support the use case where an endpoint only has a delete request defined and we don't have a read request to build the collection from. This approach flags the first generated field as a primary_key to make sure the delete-only collection is still visited during graph traversal. This solution will need to be revisited when we no longer need to flag a field as a primary_key to call the erasure on a collection (#1199).

@galvana galvana requested review from a team October 3, 2022 23:12
@galvana galvana linked an issue Oct 3, 2022 that may be closed by this pull request
@galvana galvana changed the base branch from main to unified-fides-2 October 3, 2022 23:12
@galvana galvana changed the title 1184 allow delete only saas endpoints 1184 allow delete-only saas endpoints Oct 3, 2022
@galvana galvana marked this pull request as draft October 3, 2022 23:24
@ThomasLaPiana
Copy link
Contributor

@galvana I see you requested a review, is it meant to still be in draft?

@galvana galvana removed request for a team October 4, 2022 22:57
@galvana
Copy link
Contributor Author

galvana commented Oct 4, 2022

@ThomasLaPiana getting close but not yet! I don't know how those reviewers got assigned.

@galvana galvana added the run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials label Oct 4, 2022
@galvana galvana marked this pull request as ready for review October 4, 2022 23:30
@galvana galvana requested a review from pattisdr October 4, 2022 23:30
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good solution for now that works within the existing constraints. Great code comments here too since the "why" would not always be clear without them!

Just a few questions here

src/fides/api/ops/service/connectors/saas_connector.py Outdated Show resolved Hide resolved
src/fides/api/ops/schemas/saas/saas_config.py Outdated Show resolved Hide resolved
Base automatically changed from unified-fides-2 to main October 6, 2022 16:18
@ThomasLaPiana
Copy link
Contributor

@galvana given the huge code diff (due to unified fides finally getting merged) can you open a new PR against main and cherry pick the code changes? Sorry for the trouble, I promise I'll only do this to you once 🙏

@galvana galvana force-pushed the 1184-allow-delete-only-saas-endpoints branch from 27954bc to 7f5fe8b Compare October 7, 2022 18:12
@galvana galvana requested a review from pattisdr October 7, 2022 20:03
@galvana galvana added run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials labels Oct 7, 2022
@galvana
Copy link
Contributor Author

galvana commented Oct 10, 2022

@pattisdr, this is ready for another review, the test failures are unrelated and will be addressed in #1282 and #1352

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good @galvana thanks for letting me know about the test failures

@galvana galvana merged commit 1e5e348 into main Oct 10, 2022
@galvana galvana deleted the 1184-allow-delete-only-saas-endpoints branch October 10, 2022 20:22
@galvana galvana restored the 1184-allow-delete-only-saas-endpoints branch October 12, 2022 21:17
@galvana
Copy link
Contributor Author

galvana commented Oct 12, 2022

Please refer to #1420, a commit was missed while cherry-picking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow delete-only SaaS endpoints
3 participants